Makes document attachment usage consider external attachments #1600
Conversation
Spoffy
added
Attachments
bug
|
This is dependent on the UI PR being merged. This needs to be rebased once that is merged. |
There was a problem hiding this comment.
I was wondering if we need some tests on just adding and removing attachments and checking that stats update correctly, when those attachments are external. I see some attachment accounting tests in test/nbrowser/DocumentUsage.ts in grist-saas. If those aren't entangled too much with billing would it be worth bringing them across? Or are there already tests?
app/server/lib/ActiveDoc.ts
Outdated
| const attachments = this.docData?.getMetaTable("_grist_Attachments").getRecords(); | ||
| for (const attachmentRec of attachments ?? []) { | ||
| const newSize = newFileSizesByFileIdent.get(attachmentRec.fileIdent); | ||
| if (newSize) { |
There was a problem hiding this comment.
How about checking if the size is different from the old recorded size? I believe the data engine will prune changes where a value is written to a cell that already contains that value, but seems easy to cut it out at the source here.
test/nbrowser/DocumentUsage.ts
Outdated
| import {server} from 'test/nbrowser/testServer'; | ||
| import {setupTestSuite} from 'test/nbrowser/testUtils'; | ||
|
|
||
| describe('DocumentUsage', function() { |
There was a problem hiding this comment.
Can you move this test over using DIFF? I see that you've changed the content of this file, and I don't have tools to show me the what was changed.
There was a problem hiding this comment.
I can tag which bits are the same? I've done a restructuring on this file to make testing with external attachments easier, so I'm not sure a diff would show many useful things.
There was a problem hiding this comment.
I just did a rebase that should make this easier.
This commit has all the DocumentUsage changes in:
c90b777
There was a problem hiding this comment.
New commit hash, had to do a rebase to sort a merge issue:
aacb5ee
There was a problem hiding this comment.
Can you maybe add another file for testing only external attachments and leave this one untouched?
There was a problem hiding this comment.
Most of these tests fail if untouched 😅
I can isolate and duplicate the attachments tests if you want, and ensure they pass under both external and internal.
But I'm also not seeing the harm in having a pure-core version of this file? Happy to discuss further.
There was a problem hiding this comment.
After discussion:
- Several of the non-attachments related tests will be moved to a new PR
- This file will be renamed
test/nbrowser/DocumentUsage.ts
Outdated
| async function makeSessionAndLogin() { | ||
| // login() needs an options object passing to bypass an optimization that causes .login() | ||
| // to think we're already logged in when we're not after using `server.restart()`. | ||
| // Without this we end up with bad credentials. |
There was a problem hiding this comment.
I think you end up with old credentials, not bad. You can be logged into grist using multiple accounts at the same time, this is a feature :).
There was a problem hiding this comment.
I've done a slight rephrasing.
The original session ends up with old credentials, you're right! But any new session ends up with bad credentials - specifically the bearer token ends up as api_key_for_chimpy, which just gives us the wrong responses from the API 😅
Spoffy
force-pushed
the
spoffy/attachment-size-calc
branch
from
e0a0f53 to
871b9e1
Compare
test/nbrowser/AttachmentsTransfer.ts
Outdated
There was a problem hiding this comment.
Hide whitespace changes for this diff - it'll make it much smaller. A lot of it was just indentation changes.
Spoffy
force-pushed
the
spoffy/attachment-size-calc
branch
from
6e4d36f to
b2c0483
Compare
# Conflicts: # test/nbrowser/DocumentUsage.ts
# Conflicts: # test/nbrowser/AttachmentsTransfer.ts
Spoffy
force-pushed
the
spoffy/attachment-size-calc
branch
from
b2c0483 to
bc5ae34
Compare
Context
Documents' attachment usage currently only considers files which are stored internally.
If a document has external attachments, they won't show in the document's "Size of attachments" bar in "Raw Data".
Proposed solution
This updates the attachment usage calculations to use
_grist_Attachments.fileSizefor external attachments, while preserving the existing logic for internal attachments.Missing attachments
If a document has external attachments and is uploaded to a Grist instance, these missing attachments will still count towards the document's attachment usage. There's no easy way to mitigate this in all situations without checking all attachments on document open.
Untrusted file size value
This creates a problem with imported documents, which may have an incorrect
fileSizevalue if it has been manually altered (i.e - the value is untrusted).To mitigate this, the
fileSizevalue is now updated when attachments are uploaded as a .tar archive. This means that eventually,fileSizeis always valid for files which are present in external storage.Related issues
#1021
Has this been tested?